feat: Add one-shot attribute methods to FileSystemProtocol (Fixes #3648) - #3669
feat: Add one-shot attribute methods to FileSystemProtocol (Fixes #3648)#3669wasim-builds wants to merge 4 commits into
Conversation
glbrntt
left a comment
glbrntt
left a comment
There was a problem hiding this comment.
This contains changes unrelated to the description; can you remove them? There are also no tests for the changes being made.
829a6fb to
106e3cc
Compare
|
Thanks for the review! I've removed the unrelated commit and added test cases in |
106e3cc to
ba8aeea
Compare
|
Hey @glbrntt! I've rebased the branch to remove the stacked commits — this PR now only contains the FileSystem one-shot attribute methods (with tests). Ready for another look, thanks! |
|
@glbrntt Following up on your review — Ive removed the unrelated changes and added tests covering all three one-shot attribute methods. Could you take another look when you get a chance? |
26e5b2f to
ff6c083
Compare
| public func setTimes( | ||
| forFileAt path: FilePath, | ||
| lastAccess: FileInfo.Timespec?, | ||
| lastDataModification: FileInfo.Timespec? | ||
| ) async throws { | ||
| try await self.withFileHandle(forReadingAt: path) { handle in | ||
| try await handle.setTimes(lastAccess: lastAccess, lastDataModification: lastDataModification) | ||
| } | ||
| } | ||
|
|
||
| /// Sets the file's last access time to the given time. | ||
| /// | ||
| /// - Parameters: | ||
| /// - path: The path of the file to modify. | ||
| /// - time: The time to which the file's last access time should be set. | ||
| public func setLastAccessTime( | ||
| forFileAt path: FilePath, | ||
| to time: FileInfo.Timespec | ||
| ) async throws { | ||
| try await self.setTimes(forFileAt: path, lastAccess: time, lastDataModification: nil) | ||
| } | ||
|
|
||
| /// Sets the file's last data modification time to the given time. | ||
| /// | ||
| /// - Parameters: | ||
| /// - path: The path of the file to modify. | ||
| /// - time: The time to which the file's last data modification time should be set. | ||
| public func setLastDataModificationTime( | ||
| forFileAt path: FilePath, |
There was a problem hiding this comment.
These should all be using NIOFilePath instead of FilePath
wasim-builds
left a comment
wasim-builds
left a comment
There was a problem hiding this comment.
@glbrntt I've removed the unrelated changes and added tests covering all three one-shot attribute methods. The diff is now scoped to FileSystemProtocol.swift and FileSystemTests.swift only. Could you take another look?
Address reviewer feedback on PR apple#3669: these methods should use NIOFilePath, consistent with the rest of the FileSystemProtocol API.
|
Addressed: changed |
|
@glbrntt Friendly ping! The requested changes (NIOFilePath) were implemented and pushed. Could you please take another look? |
|
Hi @glbrntt, I've addressed the requested changes: removed unrelated commits and added tests covering all three one-shot attribute methods. The current diff is scoped to FileSystemProtocol.swift and FileSystemTests.swift only. Could you please take another look when you have time? Thanks! |
Address reviewer feedback on PR apple#3669: these methods should use NIOFilePath, consistent with the rest of the FileSystemProtocol API.
df8d936 to
6b35869
Compare
6b35869 to
9a000d9
Compare
|
The requested changes have been addressed. Let me know if anything else is needed! |
9a000d9 to
32c6c7c
Compare
|
@glbrntt The requested changes to use |
Co-authored-by: glbrntt <github@glbrntt.co.uk>
|
Hey glbrntt, I've updated all the one-shot attribute methods to use NIOFilePath instead of FilePath. Thanks for catching that! |
|
Hi @glbrntt, friendly re-review request! The requested changes are already applied in the latest push:
|
|
I've removed the duplicated tests and made sure the implementation is clean and effective. Ready for review! |
2 participants
Hey @weissi! 👋
As discussed in #3648, I've added one-shot APIs to
FileSystemProtocolto make it easier to set file attributes without having to explicitly open and manage a file handle.Implementation Details:
setTimes,setLastAccessTime, andsetLastDataModificationTimeas default extension methods onFileSystemProtocol.withFileHandle(forReadingAt:)so it avoids theO_RDWRAPFS decompression penalty you mentioned.NIOFSand_NIOFileSystem.Let me know if you want any specific unit tests added or if this looks good to go! 🚀